Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Write details of container volumes to the publishing manifest #2742

Merged
merged 3 commits into from
Mar 9, 2024

Conversation

DamianEdwards
Copy link
Member

@DamianEdwards DamianEdwards commented Mar 9, 2024

Writes container volume details to the manifest. Both named and anonymous volumes are supported. Bind mounts are not written out as they're intended to mount files/directories directly from the container host machine file system into a container which isn't suitable for deployed environments.

This leaves open the possibility of modeling volumes as separate resources in the future to enable shared volumes between different containers but that's out of scope for now (aka #1521).

Related to #1676

AZD issue: Azure/azure-dev#3515

Microsoft Reviewers: Open in CodeFlow

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Mar 9, 2024
@mitchdenny
Copy link
Member

This looks good to me. The only thing I've got in the back of my head is at least thinking about how this grows into a first class volume resource. Maybe it doesn't and thats OK.

@DamianEdwards
Copy link
Member Author

DamianEdwards commented Mar 9, 2024

This looks good to me. The only thing I've got in the back of my head is at least thinking about how this grows into a first class volume resource. Maybe it doesn't and thats OK.

My thought was in the future we could expand this to support something like the following by adding a new overload of WithVolume:

var pgdata = builder.AddContainerVolume("pgdata");
var pgdb = builder.AddPostgres("pg)
    .WithVolume(pgdata)
    .AddDatabase("db");

The manifest would then be updated to support writing out a container volume that refers to another resource via a '"resource"' property or similar.

@DamianEdwards DamianEdwards enabled auto-merge (squash) March 9, 2024 01:22
@DamianEdwards DamianEdwards merged commit 1f38210 into main Mar 9, 2024
8 checks passed
@DamianEdwards DamianEdwards deleted the damianedwards/volumes-in-manifest branch March 9, 2024 01:31
@davidfowl
Copy link
Member

cc @prom3theu5

@prom3theu5
Copy link

Excellent thanks 😃
Definitely a nice feature

You say host bind mounts aren't included. What if for instance it's a Bind mount to an existing folder which contains required config, like Loki config / grafana data sources etc?

Wouldn't it be considered to be breaking if config which a service relies on at startup is missing?

For k8s I could bundle such directories as a container and use an init container to setup a pvc seeded with the data
For compose it could mount directly.

@DamianEdwards
Copy link
Member Author

You say host bind mounts aren't included. What if for instance it's a Bind mount to an existing folder which contains required config, like Loki config / grafana data sources etc?

Yeah I think there's definitely ways to make it map, but it seems even Docker sees bind mounts as a legacy and encourages use of volumes instead now. We could of course just emit the bind mounts too and let the deployment tool decide what it wants to do, but ultimately bind mounts will require extra work in deployment to actually package up the hosting directory (if it exists) in some way and get it to the target.

Please log an issue to cover emitting bind mount details into the manifest so we can explore the idea further there.

@prom3theu5
Copy link

Ok cheers - Will do 😄

@github-actions github-actions bot locked and limited conversation to collaborators Apr 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants